Should we use // +kubebuilder:validation:Enum=gzip,brotli,zstd,disabled as well?

👍 sounds good, I will add that

hi @davinci26 added this in f7b261b

Example:

$ kubectl apply -f - <<-EOF
> apiVersion: projectcontour.io/v1alpha1
kind: ContourConfiguration
metadata:
  name: example-contour-config-1
spec:
  envoy:
    listener:
      compression: bogus
EOF
The ContourConfiguration "example-contour-config-1" is invalid: spec.envoy.listener.compression: Unsupported value: "bogus": supported values: "gzip", "brotli", "zstd", "disabled"

The API here is fairly constrained so we might want to think through how would we extend it if anyone comes up with the feature request to tune the API on different compression levels

I think adding another field called Compression Strategy might be fine or create a struct that right now has a single object

Compression *EnvoyCompression `json:"compression,omitempty"`
Member

type  EnvoyCompression struct {
Aglorithm Algorithm `json:"algorithm,omitempty"
}

I did think (briefly) about making the configurability greater, and/or having a wider range of compression algorithms. In the end I decided against it, at least for this PR -

• while one could add more compression algorithms I thought that exposing the ones from envoy/extensions was a nice and easily explained choice. There are a couple of others in contrib/envoy/extensions but gzip/br/zstd seems like the ones that are most likely to be widely wanted. And I certainly didn't want to start thinking about making the algorithm choice be more "pluggable"!
• as for configurability of algorithms I thought it wasn't something I felt I wanted to start making decisions on - I expect contour project members would perhaps not want to expose all the configuration options for every algorithm, but then this leaves you with the question of how to sensibly define some configuration that supports enough configurability for the different algorithms. It felt like a bit of a rabbit hole and I thought it would be something kept separate from this PR - better to try to keep changes as small as possible, and this one is meaty enough already.

Believe it or not I actually took a bit of inspiration from the Contour Philosophy 😆 (I did read around on how to contribute), particularly the bit on Sensible Defaults. I felt these kind of gave permission to simply offer a choice of a few widely used algorithms, using default settings. While a strategy field could be added here, it would be something of a no-op feature unless we took the time to implement at least basic configurability in it. I feel this would be best left to another PR. Would that be OK?

Also now that I think of it, the feature here of most interest to us in our project is the compression disabling, so from an admittedly selfish point of view I would like to leave the configurability to a separate bit of work. Like everyone else we're very busy, and I have ended up having to do most of this work in my own time in the evenings and weekends :-(

To be transparent I dont want to design how we extend it but I am trying to think through if we leave enough space to extend the api.

In the end I decided against it, at least for this PR

My only issue is that if we go with the approach here and in 3 months we wanted to add another compression knob then we have two options:

• Make a breaking change which might be fine due to the fact that this is a v1alpha1 package.
• Add the option topline object in type EnvoyListenerConfig struct

If we think that these are fine options I think the api is good. I am just proponent of adding another struct that has a single member so we can extend just that struct

Like everyone else we're very busy, and I have ended up having to do most of this work in my own time in the evenings and weekends :-(

Yeah, a lot of the people contributing to this project to this into this capacity so I feel you.

I will defer to @projectcontour/maintainers

Ah now that you have laid it out like that I see what you mean. Let me take a look at it, hopefully this wouldn't be too large a change.

hi @davinci26 I have taken a stab at this in 5dcdc33, could you have a look and see what you think?

I still need to test this, I see some failures in e2e and will want also to run it on a local cluster, will try to get that done this week and add results in a comment.

is this from debugging?

I actually left this in here deliberately - I think it's quite nice to be able to see the choice of algorithm reflected in the initial log output. But if that's not a typical contour thing to do I'll take it out. Let me know what would be preferred.

took this out in b68f0b1

this might need update as well based on the new api

included in 5dcdc33

Let's remove the command line option and make configuration available only through config file and ContourConfiguration CRD.

The command line options currently are a bit of a mess, so we previously agreed to avoid introducing new options there unless absolutely necessary.

Would it be cleaner to treat an empty string as a valid value here, rather than completely bypassing validation in Parameters.Validate()? While algorithm is the only field currently, that might change.

Because +optionalstring in Go becomes an empty string when user did not set the field, it might be best to consider it as valid algorithm name in the Go side (CRD validation for the field can still reject it). It is already taken into account elsewhere in this change: inhttpConnectionManagerBuilder.DefaultFilters() you've added switch-default branch which will use gzip for empty algorithm string.

Minor thing but could be replaced with subtests to shorten it a bit

It seems that DefaultFilters() function used to set up a set of fixed filters - I don't see any of the other builder method impacting the default set except the new Compression() method, which now also makes it important in which order the builder methods are called - Now the configuration would be different depending if Compression() is called before or after DefaultFilters().

Maybe it would be better to (1) remove compressor from the default set completely and only add it when the new Compression() builder method is called? Or maybe some other alternative: (2) instantiate gzip by default and modify it later according to chosen algorithm when Get() is called or (3) delay adding compressor until Get() but add it by default at that point.

Compression here is still string while it should be CompressionAlgorithm. The new type needs a new table with algorithm field with the content described here.

We could add some pointers here for the user how to use it, possibly for example: